Make run.kafka.sasl.mechanism optional by aliok · Pull Request #4072 · knative/func

aliok · 2026-10-02T12:10:34Z

Changes

Follow-up to review feedback on #4069 (thanks @gauron99); reproduced on a live
cluster.

🐛 Make run.kafka.sasl.mechanism optional. func-go's Kafka runtime
defaults an empty mechanism to PLAIN, so a SASL/PLAIN broker deployed and
consumed fine before scale.keda landed. Requiring the field broke that config
on every deployer (raw, knative, keda), not only keda. Validation now
constrains only a non-empty value to the mechanisms both sides understand, and
kedaSASLType maps "" to KEDA's plaintext so the scaler authenticates the
same way the function container does.

/kind bug

Relates to #4069

Release Note

Fixed a regression where `run.kafka.sasl.mechanism` was required: it is now optional
and defaults to PLAIN, so SASL/PLAIN Kafka functions that omit it deploy again without
edits.

Docs

knative-prow · 2026-10-02T12:10:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aliok
Once this PR has been reviewed and has the lgtm label, please assign matejvasek for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Copilot review overview

🟡 Changes recommended

KEDA metadata must emit SASL plaintext configuration when the mechanism is omitted.

Review effort: Lite
Findings: 1

Open (1)

Empty SASL mechanism does not emit plaintext metadata · New

What changed in this PR

Makes run.kafka.sasl.mechanism optional, defaulting omitted values to SASL/PLAIN.

Changes:

Relaxes mechanism validation.
Maps empty mechanisms to KEDA plaintext.
Updates tests and documentation.

A critical issue remains: empty mechanisms are not emitted in KEDA ScaledObject metadata.

File	Reviewed changes
`pkg/keda/kafka_scaling.go`	Adds empty-mechanism mapping.
`pkg/keda/kafka_scaling_test.go`	Tests the mapping.
`pkg/keda/deployer_unit_test.go`	Removes obsolete required-mechanism coverage.
`pkg/functions/function.go`	Makes mechanism validation conditional.
`pkg/functions/function_test.go`	Tests empty mechanisms.
`docs/reference/func_yaml.md`	Documents the optional field and default.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🔵 Needs a closer look

Align the shipped KafkaSASL schema with validation so explicit empty mechanisms are accepted consistently.

Review effort: Lite
Findings: None

Resolved since last review (1)

Empty SASL mechanism does not emit plaintext metadata

gauron99 · 2026-10-05T08:50:08Z

-			},
-			wantErr: "run.kafka.sasl.mechanism is required",
-		},
 		{


nit: the doc comment on this test (line 355) is out of date after fa940e7. buildScaledObject now writes sasl whenever the SASL block is set, not only for a non-empty mechanism. Can you reword or drop that clause?

gauron99 · 2026-10-05T08:50:08Z

 	case "SCRAM-SHA-512":
 		return "scram_sha512"
-	case "PLAIN":
+	case "PLAIN", "":


nit (follow-up is fine): we document "defaults to PLAIN", but func never sets that default. It only works because func-go happens to use PLAIN, and seven comments repeat that fact (three in code, four in tests). A related comment already went stale in this PR (deployer_unit_test.go:355). What about one helper, e.g. KafkaSASL.EffectiveMechanism() returning "PLAIN" for "", used by the three env-wiring sites and kedaSASLType? Then the container and the scaler read the same value from func, and most of those comments can go.

gauron99 · 2026-10-05T08:50:08Z

-		triggerMeta["sasl"] = kedaSASLType(kafka.SASL.Mechanism)
+	if kafka.SASL != nil {
+		// Emit sasl whenever the SASL block is configured, not only when a
+		// mechanism is named: func-go's Kafka runtime treats a present SASL


nit: func-go never sees the SASL block. It turns SASL on from KAFKA_SECURITY_PROTOCOL (kafka/security.go). SASL != nil works here only because validation ties the block to SASL_*. Gating on SecurityProtocol would match the runtime exactly and match the tls gate just above.

gauron99 · 2026-10-05T08:50:08Z

 }

+// TestBuildScaledObject_EmptyMechanismEmitsPlaintext covers the empty-mechanism
+// SASL config this PR newly accepts: func-go defaults an omitted mechanism to


nit: "this PR" won't mean much once this merges. Maybe "an omitted mechanism" instead.

gauron99 · 2026-10-05T08:50:08Z

  - `skipVerify`: skip broker certificate verification (development only).
 - `sasl`: SASL configuration, required for `SASL_PLAINTEXT` and `SASL_SSL`.
-  - `mechanism`: one of `PLAIN`, `SCRAM-SHA-256`, `SCRAM-SHA-512`.
+  - `mechanism`: one of `PLAIN`, `SCRAM-SHA-256`, `SCRAM-SHA-512`. Optional; defaults to `PLAIN` when unset.


nit: the jsonschema description on KafkaSASL.Mechanism (pkg/functions/function.go:217) should mention the default too (e.g. "... Defaults to PLAIN."), so editor tooltips match this doc. Needs a schema regen.

fix: make run.kafka.sasl.mechanism optional

132aa97

knative-prow Bot added the kind/bug Bugs label Oct 2, 2026

knative-prow Bot requested review from dsimansk and jrangelramos October 2, 2026 12:10

knative-prow Bot added the size/M 🤖 PR changes 30-99 lines, ignoring generated files. label Oct 2, 2026

style: gofmt-align TestKedaSASLType map literal

13fa297

aliok requested a lite review from Copilot October 2, 2026 13:49

Copilot started reviewing on behalf of aliok October 2, 2026 13:50 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread pkg/keda/kafka_scaling.go

fix: emit KEDA sasl metadata for empty mechanism

fa940e7

aliok requested a lite review from Copilot October 2, 2026 14:18

Copilot started reviewing on behalf of aliok October 2, 2026 14:18 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

gauron99 reviewed Oct 5, 2026

View reviewed changes

Navigation Menu

Make run.kafka.sasl.mechanism optional - #4072

Make run.kafka.sasl.mechanism optional#4072
aliok wants to merge 3 commits into
knative:mainfrom
aliok:fix-kafka-sasl-mechanism

aliok commented Oct 2, 2026

Uh oh!

knative-prow Bot commented Oct 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

gauron99 Oct 5, 2026

Uh oh!

gauron99 Oct 5, 2026

Uh oh!

gauron99 Oct 5, 2026

Uh oh!

gauron99 Oct 5, 2026

Uh oh!

gauron99 Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aliok commented Oct 2, 2026

Changes

Uh oh!

knative-prow Bot commented Oct 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

gauron99 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

gauron99 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants